Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove files from cloud storage when version is wiped. #6186

Closed
wants to merge 9 commits into from
Closed

Remove files from cloud storage when version is wiped. #6186

wants to merge 9 commits into from

Conversation

dojutsu-user
Copy link
Member

Fixes #6069

@dojutsu-user dojutsu-user added the PR: work in progress Pull request is not ready for full review label Sep 16, 2019
@dojutsu-user
Copy link
Member Author

dojutsu-user commented Sep 16, 2019

Should we add a FAQ question for this?
Something like -- How can I remove old results from the search results?

Edit: Added the FAQ.

@dojutsu-user dojutsu-user removed the PR: work in progress Pull request is not ready for full review label Sep 17, 2019
@stsewd
Copy link
Member

stsewd commented Sep 17, 2019

Should we add a FAQ question for this?

Not sure about that, if the files aren't refreshed that seems like a bug from our side.

readthedocs/core/utils/general.py Outdated Show resolved Hide resolved
readthedocs/core/utils/general.py Show resolved Hide resolved
readthedocs/core/utils/general.py Outdated Show resolved Hide resolved
Copy link
Contributor

@davidfischer davidfischer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. I agree with @stsewd's suggestions, however, that any renaming of functions should probably be in a separate PR if at all. Let's make this PR just about removing files from media storage.

readthedocs/core/utils/general.py Outdated Show resolved Hide resolved
readthedocs/core/utils/general.py Outdated Show resolved Hide resolved
-----------------------------------------------

If the search results contains stale/outdated results,
you can update the search index by :doc:`wiping the build environment <guides/wipe-environment>` and then rebuilding your docs.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this. I mean, if that is happening I would like that users report that to us, since it is a bug from our side. We could remove this or add something like if this is happening very often, report it.

@davidfischer what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stsewd
I think this is a bug from Sphinx, if a .rst file is deleted, then the .html file for that shouldn't be created or deleted in the next build.

I think it would be better if a user tries wiping the environment and rebuilding the docs, if that doesn't work -- then report to us.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, as we move toward each build being totally fresh and not relying on a previous environment, this problem should naturally go away. It is possible that we need to detect when files aren't generated as part of the build and remove them. Even with this change, that won't be done exactly.

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change is not correct. Why? Because it changes the meaning of a Wipe.

Before this PR, the Wipe action only takes action on the builders (deleting some cached files). After this PR, if I understand correctly, the Wipe will also remove the live documentation for that version.

@jcampbell
Copy link

@humitos : what do you think should be the way to trigger the removal from cloud storage? As someone following this issue from outside, the primary issue for me is that I have a bad search index in cloud storage and no way at all to fix it.

@humitos
Copy link
Member

humitos commented Oct 9, 2019

I'm not proposing a solution for the issue. I'm not sure how it should be.

I'm saying that the proposed solution in this PR could introduce a behavior (a bug, to me) that we don't want.

I may be wrong, though.

@stsewd
Copy link
Member

stsewd commented Oct 9, 2019

@humitos I think you may be right. Then I think the problem is something similar to what we had in .com with the indexes. We are reading the json objects from the wrong directory.

@stsewd
Copy link
Member

stsewd commented Oct 9, 2019

And also, maybe we are not cleaning the old files from storage before a new build as we do with the file based storage

@dojutsu-user
Copy link
Member Author

I think that @davidfischer can provide more info here.

@davidfischer
Copy link
Contributor

I think that @davidfischer can provide more info here.

Credit to @stsewd but I think the issue is that with filesystem backed storage, we were running rsync which both copied new/updated files and deleted removed files. The current storage system doesn't do that with cloud storage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search index not updated after renaming/moving of a page
5 participants